Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Lens] Consistent Drag and Drop styles #78674

Merged
merged 27 commits into from
Oct 1, 2020
Merged

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Sep 28, 2020

Closes #75938 & closes #78643

The biggest change is that instead of the DnD component wrapping it's children in another layer, it now clones it and just applies the extra classes and handlers. This enabled me to provide the classes at the appropriate levels (closes to the button).

And they styles now all have the same coloring, transition timings, etc. It also moved a lot of the logic of copy vs move to the DnD component itself so that we can be consistent moving forward.

Screen Shot 2020-09-28 at 15 41 52 PM

GIF: https://share.getcloudapp.com/jkuDrzJQ


Other quick fixes:

  1. Min-height for the workspace area
  2. Using EuiHighlight for the search term highlighting in the fields panel instead of the custom UI

Checklist

Delete any items that are not applicable to this PR.

Copy link
Contributor Author

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@flash1293 Not much needed for help here except for one test and one prop that is not updating with DnD

x-pack/plugins/lens/public/drag_drop/drag_drop.tsx Outdated Show resolved Hide resolved
@flash1293
Copy link
Contributor

@elasticmachine merge upstream

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed a few things, I will fix the update problem of the dragdrop component tomorrow. For the other things commented on below I think you know than me already :-)

@flash1293
Copy link
Contributor

@cchaos I merged master and implemented the fix with a custom equality check function. I had to change the logic for applying classnames a little, otherwise css properties would get overwritten. It should work fine now:
Screenshot 2020-09-30 at 10 47 55

Can you take another look whether everything still works as you intended?

@cchaos
Copy link
Contributor Author

cchaos commented Sep 30, 2020

Thanks @flash1293. It's all working and looking great now. I'll pull out of drafts.

@cchaos cchaos marked this pull request as ready for review September 30, 2020 13:39
@cchaos cchaos requested a review from a team September 30, 2020 13:39
@cchaos cchaos requested a review from a team as a code owner September 30, 2020 13:39
@cchaos cchaos requested a review from a team as a code owner September 30, 2020 13:39
@cchaos cchaos added Feature:Lens v7.10.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Sep 30, 2020
Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pushed a fix to fix an SCSS failure I found:

SassError: SassError: no mixin named lnsDroppable
                  on line 70 of /Users/wylie/dev/kibana/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel_wrapper.scss
          >>     @include lnsDroppable;

I'll be continuing to review the code and behavior.

Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed and tested in Chrome and Firefox. The new changes LGTM, including performance of the Eui highlight on large index patterns. I found that there is an existing styling issue in the drag styling for the Filters and Ranges editors:

drag styling issue

@@ -27,36 +27,41 @@

.lnsLayerPanel__dimension {
@include euiFontSizeS;
background: lightOrDarkTheme($euiColorEmptyShade, $euiColorLightestShade);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was a bit surprised to see how low-contrast the empty dimensions look now, with gray on gray text. Would you consider choosing a darker text color or lighter background color, or do you think the low contrast is effective here?

Screen Shot 2020-09-30 at 2 13 31 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the low contrast here was intentional as an additional support of #76021 and as indicated in the mocks in #75938. It further helps distinguish between filled and unfilled dimensions. The contrast itself is still sufficient to meet a11y.
Screen Shot 2020-09-30 at 14 21 41 PM

@cchaos
Copy link
Contributor Author

cchaos commented Sep 30, 2020

I found that there is an existing styling issue in the drag styling for the Filters and Ranges editors

I actually see that more as a feature than an issue because this particular DnD interaction is restricted to it's own panel while the others aren't. So it helps reinforce this containment.

@cchaos
Copy link
Contributor Author

cchaos commented Oct 1, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
lens 1002.8KB 1007.4KB +4.6KB

page load bundle size

id before after diff
kibanaReact 123.3KB 124.4KB +1.0KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@cchaos cchaos merged commit e248f32 into elastic:master Oct 1, 2020
@wylieconlon wylieconlon added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Oct 1, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

cchaos added a commit that referenced this pull request Oct 1, 2020
* Remove wrapping div of DragDrop and pass props to child
* Using EuiHighlight
* Basic styles in for all DnD states
* Fixing dimension button styles
* Fix FieldButton to accept `…rest` props
* A few other minor fixes
* Fixed horizontal scroll of error message
* Quick fix for invalid link
@cchaos cchaos deleted the lens/dnd-design branch October 1, 2020 18:48
phillipb added a commit to phillipb/kibana that referenced this pull request Oct 1, 2020
…aly-detection-partition-field

* 'master' of github.com:elastic/kibana: (76 commits)
  Fix z-index of KQL Suggestions dropdown (elastic#79184)
  [babel] remove unused/unneeded babel plugins (elastic#79173)
  [Search] Fix timeout upgrade link (elastic#79045)
  Always Show Embeddable Panel Header in Edit Mode (elastic#79152)
  [Ingest]: add more test for transform index (elastic#79154)
  [ML] DF Analytics: Collapsable sections on results pages (elastic#76641)
  [Fleet] Fix agent policy change action migration (elastic#79046)
  [Ingest Manager] Match package spec `dataset`->`data_stream` and `config_templates`->`policy_templates` renaming (elastic#78699)
  Revert "[Metrics UI] Add ability to override datafeeds and job config for partition field (elastic#78875)"
  [ML] Update transform cloning to include description and new fields (elastic#78364)
  chore(NA): remove non existing plugin paths from case api integration tests (elastic#79127)
  [Ingest Manager] Ensure we trigger agent policy updated event when we bump revision. (elastic#78836)
  [Metrics UI] Display No Data context.values as [NO DATA] (elastic#78038)
  [Monitoring] Missing data alert (elastic#78208)
  [Lens] Fix embeddable title and description for reporting and dashboard tooltip (elastic#78767)
  [Lens] Consistent Drag and Drop styles (elastic#78674)
  [ML] Model management UI fixes and enhancements (elastic#79072)
  [Metrics UI] Add ability to override datafeeds and job config for partition field (elastic#78875)
  [Security Solution]Fix basepath used by endpoint telemetry tests (elastic#79027)
  update rum agent version which contains longtasks (elastic#79105)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.10.0 v8.0.0
Projects
None yet
5 participants